Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a proper LSP logger to improve debugging visibility in LSP clients that don't display console output. The implementation introduces a logging system that sends messages via the LSP window/logMessage protocol, replacing direct console.log/error calls in the incremental compilation and project configuration caching code paths.
Key Changes:
- Added a new
logger.tsmodule with a Logger interface and LSP-compliant implementation that supports configurable log levels (error, warn, info, log) - Replaced conditional debug logging with unconditional logger calls that respect the configured log level
- Added
logLevelconfiguration option withincrementalTypechecking.debugLoggingas a legacy override
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/logger.ts | New file implementing Logger interface with LSP-compliant logging via window/logMessage protocol |
| server/src/server.ts | Added logger initialization, applyUserConfiguration function to handle log level settings, and migrated console.log/error calls to logger |
| server/src/incrementalCompilation.ts | Removed debug() function and migrated all debug-gated console logs to logger calls |
| server/src/config.ts | Exported initialConfiguration and added logLevel to extensionConfiguration interface |
| server/src/bsc-args/rewatch.ts | Migrated debug-gated console.log calls to logger |
| package.json | Added rescript.settings.logLevel configuration with enum values including error, warn, info, log, and debug |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export interface Logger { | ||
| error(message: string): void; | ||
| warn(message: string): void; | ||
| info(message: string): void; | ||
| log(message: string): void; | ||
| } |
There was a problem hiding this comment.
The Logger interface and its implementations lack documentation. Consider adding JSDoc comments to explain the purpose of the logger, the log level hierarchy (log is most verbose, error is least verbose), and when each log level should be used. This will help other developers understand how to use the logger correctly.
zth
left a comment
zth
left a comment
There was a problem hiding this comment.
Looks good to me. Just a few questions.
2 participants
Writing to the console does not show up in all LSP clients.
I was pretty blind in Zed when something did not work as expected.
Turned out my problem was that we do not override the default settings when the client provides them (see 7334e1e).
configuration.incrementalTypechecking?.debugLoggingenables the highest log level.This is to semantically close enough to what we have today. Existing config won't break.